Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to use rebar3 fmt instead of standalone erlfmt #4811

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yosida95
Copy link

The current implementation of the fixer for erlfmt, an Erlang code formatter, invokes a standalone erlfmt command. However, the standalone erlfmt command is not distributed by default. Instead, erlfmt is often used as a plugin for Rebar3, the de facto build tool for Erlang.

This patch introduces a new option g:ale_erlang_erlfmt_use_rebar, which is disabled by default, allowing the use of rebar3 fmt instead.

Additionally, this patch fixes a bug that caused the command to read the original source code rather than the stdin.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good! See my comment here.

call ale#Set('erlang_rebar_executable', 'rebar3')
call ale#Set('erlang_rebar_use_global', get(g:, 'ale_use_global_executables', 0))

function! ale#erlang#GetRebarExecutable(buffer) abort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a runtime cost in loading this file, and we only have this one function and call it from one place. Let's just move this into the fixer file instead, to avoid this runtime cost.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@w0rp w0rp self-assigned this Aug 2, 2024
@w0rp w0rp added this to the Version 4.0.0 milestone Aug 2, 2024
@yosida95
Copy link
Author

yosida95 commented Aug 2, 2024

@w0rp Thank you for your feedback! I have addressed the points you raised. Could you please review the changes again?

@yosida95 yosida95 requested a review from w0rp October 10, 2024 05:35
let l:executable = ale#fixers#erlfmt#GetExecutable(a:buffer)
let l:use_rebar = ale#Var(a:buffer, 'erlang_erlfmt_use_rebar')

if l:use_rebar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be clearer if the two cases were handled in separate functions, like this:

function! ale#fixers#erlfmt#Fix(buffer) abort
    return ale#Var(a:buffer, 'erlang_erlfmt_use_rebar3')
    \   ? s:FixWithRebar3(a:buffer)
    \   : s:FixWithEscript(a:buffer)
endfunction

However, it seems to me that from user's perspective it would be better to add separate rebar3_fmt fixer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants